Add support for cryptex #511
Conversation
|
hi, it would be good to get the CI build passing as soon as possible, are you able to fix that or doe you need some help? |
|
Great to have this on a branch but I would like to see the IETF draft to get further along before we merge it to the main branch. |
|
@fluffy I agree. My main intent was to get feedback on the changes and spot any issue on the implementation that could impact the spec. Would be great if it could be thoroughly reviewed so we can confirm that the test vectors can be incorporated into the rfc and be ready to merge the branch as soon as the rfc is ready. |
|
Given the IETF draft is about at WGLC, we should merge this in once we are happy with the tests. |
@murillo128 are you able to fix the compile issues? Maybe rebase on master as well? @paulej have you thought to review these changes ? |
|
I'll do it later this month when I am back from vacations |
paulej
left a comment
paulej
left a comment
There was a problem hiding this comment.
Looks good. I appreciate that there is test code to go with the implementation, because I'll admit I didn't check every field to ensure the math is right.
backport changes from master
|
I have also rebased to master and fixed the CI errors of the previous version. Could anyone run the github workflows to check if everything is correct now? |
|
fixed format and mem leak on deallocating recv sessions on tests |
|
I managed to get it to fully work with gcm and openssl, cryptex in this mode also requires multiple calls to srtp_cipher_set_aad but it seamed to work. Will look into the other backends. |
|
@murillo128, I have updated the code in #724, I have abandoned the attempt to support calling encrypt/decrypt multiple times when using a gcm cipher as it is not well support by the different backends. Can look in to this again later. So the current state is that if the io is in-place and there are CC's in the header then the input buffer will be rearranged similar to your original PR, this is supported by both icm and gcm. If there are CC's in the header and io is not in-place then there will multiple calls to encrypt or decrypt, this is only supported by icm. If there are no CC's in the header then the start offset is just updated and there is a single call to encrypt or decrypt, this is supported by both icm and gcm. Short summary is if you want to use cryptex then you should use in-place io. Do you think could merge that code in to this PR and it could get reviewed and hopefully merged? |
|
@murillo128 have you had a chance to look at my proposal? I am thinking to merge it in to your branch soon so it can be reviewed and tested. |
Move cryptex code to separate functions so it can be reused. Due to support for non in-place io it is not always possible to modify the input buffer in the way the cryptex draft expected therefore use multiple encrypt / decrypt calls when io is not in-place. The gcm ciphers do not currently support multiple operations so non in-place io with csrc's is not supported.
|
@murillo128 I have force pushed my changes to your branch, overwriting your recent merging with main attempt, I hope that is ok. I have made a local copy of your changes if you would like it reverted. It would be could to get this reviewed again and then it can be merged. There are some limitations as described earlier but I think it is acceptable for now. |
|
Hi @JonathanLennox, |
Verify that an error is return in this invalid use case. Also fix up based on review.
| srtp_err_status_pkt_idx_adv = 27, /**< packet index advanced, reset */ | ||
| /**< needed */ | ||
| srtp_err_status_buffer_small = 28, /**< out buffer is too small */ | ||
| srtp_err_status_cryptex_err = 29, /**< unsupported cryptex operation */ |
There was a problem hiding this comment.
Rather than adding this error, why not just disable extension encryption of extensions are not present?
There was a problem hiding this comment.
Want to avoid people thinking the csrc's will be protected but are actually not as they did not include a header extension, unless I misunderstood you comment.
There was a problem hiding this comment.
OK, that's a valid argument. If the only reason for the error is that extension encryption is requested, but extensions were not given, perhaps that should be in the description. Like, "extensions required for cryptex".
|
Which libsrtp version will be the first one to include cryptex support? I don't see it referenced in any of the |
|
@lminiero This was just merged into main 3 weeks ago. It would be targeted for a 3.x release. |
|
Thanks! |
|
What is the best way to conditionally enable this at compile-time? |
|
@fippo why would you want to conditionally enable at compile time? It is a config option when creating a session, it should have no effect if not enabled. |
|
(this fell through the cracks) What I meant was "how do I determine if cryptex is available so I can ifdef the calls to enable it at compile-time" |
|
OK now I understand, I guess there is no obvious way, What do suggest? |
Add support for cryptex
10 participants
Implementation of cryptex as per https://github.com/juberti/cryptex
Discussion:
juberti/cryptex#5